#620 Monitoring – OTEL, Grafana, graphs and alerting - #753
Conversation
- Introduced metrics for tracking cluster leader presence, node leadership status, and partition counts. - Added metrics for job lifetime, process instance duration, and message correlation failures. - Enhanced the monitoring stack with Prometheus configuration for alerting.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds OpenTelemetry tracing and metrics instrumentation across BPMN/DMN engines, cluster/partition/store components, and job dispatch. It adds health/readiness endpoints, sampler-ratio validation, span attribute renaming, and a local monitoring stack (Prometheus, Alertmanager, Grafana dashboards) with documentation. ChangesZenBPM observability
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RESTServer
participant ZenNode
participant Store
Client->>RESTServer: GET /system/health/ready
RESTServer->>ZenNode: Health()
ZenNode->>Store: check leadership and initialization
Store-->>ZenNode: state snapshot
ZenNode-->>RESTServer: healthy bool, sorted reasons
RESTServer->>Client: writeHealthResponse (200 UP or 503 DOWN)
sequenceDiagram
participant JobmanagerServer
participant EngineBatch
participant EngineMetrics
participant DmnEngine
JobmanagerServer->>JobmanagerServer: distribute job, record JobsDistributed and JobActivationLatency
EngineBatch->>EngineBatch: flush timer/incident post-flush actions
EngineBatch->>EngineMetrics: record incident/timer/job lifetime metrics
DmnEngine->>DmnEngine: evaluateDRD records span and duration histogram
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/bpmn/jobs_api.go (1)
89-97: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
recordJobLifetime(..., "failed")also fires when the job was caught by an error boundary/subprocess, not genuinely failed.This defer only checks
retErr == nil, butJobFailByKeyreturnsnilearly both when the job is redirected to an error-boundary event (line ~111) or an error event-subprocess (line ~117), and when it's actually failed viafailJobWithIncident(line ~127). Since the same defer fires on everynil-returning path,engine.metrics.JobsFailedand the newengine.recordJobLifetime(ctx, job, "failed")both misclassify caught/handled jobs as "failed", skewing the outcome-labeled telemetry this PR introduces.Consider tracking whether the job was actually handled by a catch target and skipping the "failed" metrics in that case, or moving the metric calls to sit alongside the genuine
failJobWithIncidentsuccess path instead of the blanket defer.🐛 Sketch of a fix
+ handled := false defer func() { - if retErr == nil { + if retErr == nil && !handled { engine.metrics.JobsFailed.Add(ctx, 1, metric.WithAttributes( attribute.String("type", job.Type), attribute.Bool("internal", false), )) engine.recordJobLifetime(ctx, job, "failed") } }() if errorCode != nil { target, err := engine.findErrorCatchTarget(ctx, &batch, instance, job.Token, errorCode) if err != nil { return err } if target != nil { switch { case target.boundary != nil: if handledBoundary, err := engine.processBoundaryErrorEvent(ctx, &batch, job, instance, target.boundary, variables); err != nil { return err } else if handledBoundary { + handled = true return nil } case target.eventSubprocess != nil: if handledSub, err := engine.processErrorEventSubprocessForJob(ctx, &batch, job, target.eventSubprocess, variables); err != nil { return err } else if handledSub { + handled = true return nil } } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/bpmn/jobs_api.go` around lines 89 - 97, Update JobFailByKey so caught jobs are not classified as failed: distinguish error-boundary and error-event-subprocess handling from the genuine failJobWithIncident path, and emit engine.metrics.JobsFailed and engine.recordJobLifetime(ctx, job, "failed") only for actual failures. Remove or adjust the blanket retErr-based defer while preserving existing handling behavior.
🧹 Nitpick comments (3)
pkg/bpmn/engine_batch.go (1)
148-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated save+metric-scheduling pattern across three methods.
WriteTokenIncident,WriteMessageIncident, andSaveIncidenteach independently implement "save incident, then on success append apostFlushActionscallback callingrecordIncidentMetric";SaveTimerrepeats the analogous shape forrecordTimerMetric. Consider extracting a small shared helper (e.g.saveIncidentWithMetric(ctx, incident) error) to reduce duplication and keep the persistence+metric contract in one place.Also applies to: 166-201, 219-227, 257-265
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/bpmn/engine_batch.go` around lines 148 - 165, Extract the repeated incident persistence and metric scheduling logic from WriteTokenIncident, WriteMessageIncident, and SaveIncident into a shared EngineBatch helper such as saveIncidentWithMetric(ctx, incident) error. Have the helper save the incident, log and return persistence errors, and append the postFlushActions callback that invokes recordIncidentMetric only after a successful save; update each caller to use it while preserving existing behavior. Apply the same deduplication principle to SaveTimer for recordTimerMetric only if a corresponding shared timer helper is already supported by the surrounding code.pkg/otel/traces.go (1)
4-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Go initialisms and add the missing doc comment.
AttributeProcessId,AttributeElementId,AttributeDecisionId, andAttributeDrdIdshould useID, andAttributeProcessInstanceKeyneeds its own doc comment on this exported API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/otel/traces.go` around lines 4 - 27, Update the exported constants AttributeProcessId, AttributeElementId, AttributeDecisionId, and AttributeDrdId to use Go initialism naming with ID, preserving their existing attribute values. Add a dedicated doc comment immediately before AttributeProcessInstanceKey describing its purpose.Source: Linters/SAST tools
scripts/prometheus.yml (1)
1-11: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAlert latency:
evaluation_intervalunset defaults to 1m, underminingfor: 30salerts.Several critical alerts (
NoClusterLeader,NoPartitionLeader) usefor: 30s, but Prometheus rule evaluation defaults toevaluation_interval: 1mwhen not set (it doesn't inheritscrape_interval). Detection latency for these leader-loss alerts will effectively be governed by the 1m evaluation cadence rather than 30s.⏱️ Suggested fix
global: scrape_interval: 5s + evaluation_interval: 5s🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prometheus.yml` around lines 1 - 11, Set the Prometheus global evaluation_interval alongside scrape_interval in the configuration, using a cadence no longer than the 30s alert duration so NoClusterLeader and NoPartitionLeader rules are evaluated promptly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/prometheus-rules.yml`:
- Around line 24-31: Update the dashboard panel in
scripts/grafana_provisioning/dashboards/zenbpm/cluster.json at lines 37-43 to
use max by(partition) (partition_has_leader), matching the NoPartitionLeader
alert expression. Leave scripts/prometheus-rules.yml lines 24-31 unchanged
because its NoPartitionLeader expression already uses the required aggregation.
---
Outside diff comments:
In `@pkg/bpmn/jobs_api.go`:
- Around line 89-97: Update JobFailByKey so caught jobs are not classified as
failed: distinguish error-boundary and error-event-subprocess handling from the
genuine failJobWithIncident path, and emit engine.metrics.JobsFailed and
engine.recordJobLifetime(ctx, job, "failed") only for actual failures. Remove or
adjust the blanket retErr-based defer while preserving existing handling
behavior.
---
Nitpick comments:
In `@pkg/bpmn/engine_batch.go`:
- Around line 148-165: Extract the repeated incident persistence and metric
scheduling logic from WriteTokenIncident, WriteMessageIncident, and SaveIncident
into a shared EngineBatch helper such as saveIncidentWithMetric(ctx, incident)
error. Have the helper save the incident, log and return persistence errors, and
append the postFlushActions callback that invokes recordIncidentMetric only
after a successful save; update each caller to use it while preserving existing
behavior. Apply the same deduplication principle to SaveTimer for
recordTimerMetric only if a corresponding shared timer helper is already
supported by the surrounding code.
In `@pkg/otel/traces.go`:
- Around line 4-27: Update the exported constants AttributeProcessId,
AttributeElementId, AttributeDecisionId, and AttributeDrdId to use Go initialism
naming with ID, preserving their existing attribute values. Add a dedicated doc
comment immediately before AttributeProcessInstanceKey describing its purpose.
In `@scripts/prometheus.yml`:
- Around line 1-11: Set the Prometheus global evaluation_interval alongside
scrape_interval in the configuration, using a cadence no longer than the 30s
alert duration so NoClusterLeader and NoPartitionLeader rules are evaluated
promptly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b9a7b5a-1a23-4ec1-b08e-35bb1f4e7e0b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
Makefiledocs/reference/observability.mdgo.modinternal/cluster/jobmanager/otel.gointernal/cluster/jobmanager/server.gointernal/cluster/node.gointernal/cluster/partition/partition.gointernal/cluster/partition/partition_persistence.gointernal/cluster/store/metrics.gointernal/cluster/store/store.gointernal/cluster/store/store_helper.gointernal/config/config.gointernal/otel/metrics.gointernal/otel/tracer.gointernal/otel/tracer_test.gointernal/rest/health_test.gointernal/rest/server.gopkg/bpmn/engine.gopkg/bpmn/engine_api.gopkg/bpmn/engine_batch.gopkg/bpmn/engine_metrics_test.gopkg/bpmn/events_api.gopkg/bpmn/jobs_api.gopkg/bpmn/start_event_instance_creation_handler.gopkg/dmn/dmn_engine.gopkg/dmn/dmn_multiline_test.gopkg/otel/metrics.gopkg/otel/traces.goscripts/alertmanager.ymlscripts/grafana_provisioning/dashboards/zenbpm/cluster.jsonscripts/grafana_provisioning/dashboards/zenbpm/host.jsonscripts/grafana_provisioning/dashboards/zenbpm/incidents.jsonscripts/grafana_provisioning/dashboards/zenbpm/latency.jsonscripts/grafana_provisioning/dashboards/zenbpm/storage.jsonscripts/prometheus-rules.ymlscripts/prometheus.yml
- fixes for CI auto AI findings: update OpenTelemetry attribute names for consistency and improve Grafana query for partition leaders
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/grafana_provisioning/dashboards/zenbpm/cluster.json (2)
50-51: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not sum the per-node leader-change counter.
partition_leader_changesis recorded as an event count observed by each node.sum by(partition)counts the same event once per node. Select one authoritative series, or usemax by(partition)when every node observes the same events.Proposed query change
- "expr": "sum by(partition) (increase(partition_leader_changes_total[5m]))" + "expr": "max by(partition) (increase(partition_leader_changes_total[5m]))"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/grafana_provisioning/dashboards/zenbpm/cluster.json` around lines 50 - 51, Update the dashboard target query for partition leader changes to avoid summing duplicated per-node observations; replace sum by(partition) with an authoritative single series or max by(partition), while preserving the existing increase window, partition grouping, legendFormat, and refId.
21-22: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse current Prometheus samples for status panels.
If metric collection stops,
lastNotNullcan display an oldLEADER OKvalue across the one-hour range. Set"instant": trueon both stat targets and configure a distinct no-data display.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/grafana_provisioning/dashboards/zenbpm/cluster.json` around lines 21 - 22, Update both status panel targets in the dashboard JSON to use current Prometheus samples by setting instant querying on each target, and configure a distinct no-data display for the stat panels. Preserve the existing expressions, legends, and references while applying the same behavior consistently to both targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/grafana_provisioning/dashboards/zenbpm/cluster.json`:
- Line 40: Update the dashboard panel using the partition_has_leader target so
expected partitions absent from cluster state remain visible and display NO
LEADER. Compare cluster_partitions with cluster_desired_partitions, or otherwise
add zero-valued series for missing expected partitions, while preserving the
existing leader status display for present partitions.
---
Outside diff comments:
In `@scripts/grafana_provisioning/dashboards/zenbpm/cluster.json`:
- Around line 50-51: Update the dashboard target query for partition leader
changes to avoid summing duplicated per-node observations; replace sum
by(partition) with an authoritative single series or max by(partition), while
preserving the existing increase window, partition grouping, legendFormat, and
refId.
- Around line 21-22: Update both status panel targets in the dashboard JSON to
use current Prometheus samples by setting instant querying on each target, and
configure a distinct no-data display for the stat panels. Preserve the existing
expressions, legends, and references while applying the same behavior
consistently to both targets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 46da2e47-8920-4a4d-9918-914db1e87154
📒 Files selected for processing (11)
internal/cluster/node.gointernal/cluster/partition/partition_persistence.gointernal/cluster/store/metrics.gointernal/rest/server.gopkg/bpmn/engine.gopkg/bpmn/engine_api.gopkg/bpmn/multi_instance.gopkg/bpmn/sub_process.gopkg/dmn/dmn_engine.gopkg/otel/traces.goscripts/grafana_provisioning/dashboards/zenbpm/cluster.json
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/cluster/node.go
- pkg/bpmn/engine.go
- internal/cluster/partition/partition_persistence.go
- pkg/dmn/dmn_engine.go
- internal/cluster/store/metrics.go
- pkg/bpmn/engine_api.go
- feat(monitoring): add partition deficit metric to Grafana dashboard
There was a problem hiding this comment.
Pull request overview
Adds a complete local-first observability stack for ZenBPM (Prometheus + Alertmanager + Grafana + OTEL tracing/metrics), alongside new health probes and a documented metrics/alerting catalog.
Changes:
- Introduces Prometheus alert rules + Alertmanager config and multiple Grafana dashboards provisioned for ZenBPM metrics.
- Expands OTEL instrumentation across BPMN/DMN engines, cluster/partition/raft, and rqlite persistence (plus runtime metrics).
- Adds liveness/readiness health endpoints and corresponding tests + observability reference documentation.
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/prometheus.yml | Enables rule loading and Alertmanager integration for the local Prometheus container. |
| scripts/prometheus-rules.yml | Adds technical + business Prometheus alerting rules for cluster health and engine signals. |
| scripts/alertmanager.yml | Provides a default “blackhole” Alertmanager config for local use. |
| scripts/grafana_provisioning/dashboards/zenbpm/storage.json | Grafana dashboard for rqlite size/growth, latency, and disk free. |
| scripts/grafana_provisioning/dashboards/zenbpm/latency.json | Grafana dashboard for latency percentiles and throughput. |
| scripts/grafana_provisioning/dashboards/zenbpm/incidents.json | Grafana dashboard for incidents, errors, timers, message correlation, DMN. |
| scripts/grafana_provisioning/dashboards/zenbpm/host.json | Grafana dashboard for node_exporter host resource metrics. |
| scripts/grafana_provisioning/dashboards/zenbpm/cluster.json | Grafana dashboard for raft/cluster leadership and partition health. |
| pkg/otel/traces.go | Renames ZenBPM tracing attribute keys into a zenbpm.* namespace. |
| pkg/otel/metrics.go | Adds shared latency buckets + new engine metrics instruments (incidents, timers, durations, message correlation). |
| pkg/dmn/dmn_multiline_test.go | Updates DMN tests to pass context.Context. |
| pkg/dmn/dmn_engine.go | Adds DMN tracing + evaluation counters/histograms around decision evaluation. |
| pkg/bpmn/sub_process.go | Updates span attributes to the renamed OTEL attribute constants. |
| pkg/bpmn/start_event_instance_creation_handler.go | Records timer-fired metrics for definition-level timer start events post-flush. |
| pkg/bpmn/multi_instance.go | Updates span attributes to the renamed OTEL attribute constants. |
| pkg/bpmn/jobs_api.go | Records job lifetime histogram on completion/failure. |
| pkg/bpmn/events_api.go | Records message correlation success/failure counters with bounded label values. |
| pkg/bpmn/engine.go | Records definition-level timer lifecycle metrics when bypassing EngineBatch.SaveTimer. |
| pkg/bpmn/engine_metrics_test.go | Adds tests for the new engine metric recorders. |
| pkg/bpmn/engine_batch.go | Adds post-flush incident/timer metric recording and implements incident/timer recorder helpers. |
| pkg/bpmn/engine_api.go | Records process instance duration histogram on instance completion/failure. |
| Makefile | Extends start-monitoring to run Alertmanager and mount rule files. |
| internal/rest/server.go | Adds /system/health/live + /system/health/ready and improves /system/status error handling. |
| internal/rest/health_test.go | Adds unit tests for the health response helper. |
| internal/otel/tracer.go | Adds sampler ratio validation and configures parent-based ratio sampling. |
| internal/otel/tracer_test.go | Tests sampler ratio validation and fail-fast behavior. |
| internal/otel/metrics.go | Adds global propagators and runtime metrics instrumentation; validates sampler ratio early. |
| internal/config/config.go | Adds tracing sampler ratio configuration (env + yaml/json). |
| internal/cluster/store/store.go | Stores OTEL callback registration handle for later cleanup. |
| internal/cluster/store/store_helper.go | Unregisters OTEL callbacks on store close to avoid leaks. |
| internal/cluster/store/metrics.go | Adds observable gauges for main cluster raft/partition health. |
| internal/cluster/partition/partition.go | Adds partition-level gauges/counters (leadership, db size, leader changes). |
| internal/cluster/partition/partition_persistence.go | Records rqlite exec/query histograms alongside trace spans. |
| internal/cluster/node.go | Registers store metrics and adds a readiness Health() evaluation. |
| internal/cluster/jobmanager/server.go | Records job activation latency histogram on successful distribution. |
| internal/cluster/jobmanager/otel.go | Registers job_activation_latency histogram and fixes joined error wrapping. |
| go.mod | Adds OTEL runtime instrumentation dependency. |
| go.sum | Adds checksums for OTEL runtime instrumentation dependency. |
| docs/reference/observability.md | Adds comprehensive observability documentation: endpoints, tracing, metrics catalog, alerts, dashboards. |
Suppressed comments (1)
pkg/bpmn/engine_batch.go:298
recordTimerMetriccan panic ifotel.NewMetricsreturned an error but the engine kept the partially-initialized metrics struct (i.e. one of the timer counters is nil). Guard each counter before callingAdd.
engine.metrics.TimersScheduled.Add(ctx, 1)
case bpmnruntime.TimerStateTriggered:
engine.metrics.TimersFired.Add(ctx, 1)
case bpmnruntime.TimerStateCancelled:
engine.metrics.TimersCancelled.Add(ctx, 1)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| Endpoint string `yaml:"endpoint" env:"OTEL_EXPORTER_OTLP_ENDPOINT"` | ||
| // SamplerRatio controls the fraction of new traces that get sampled (0.0 - 1.0). | ||
| // Child spans follow the sampling decision of their parent (ParentBased sampler). | ||
| SamplerRatio float64 `yaml:"samplerRatio" json:"samplerRatio" env:"TRACING_SAMPLER_RATIO" env-default:"1.0"` |
There was a problem hiding this comment.
Please add the tracing prefix here as well, to make it clear that it belongs to tracing.
There was a problem hiding this comment.
Good catch — the TRACING_SAMPLER_RATIO env var already carries the prefix, but I found the actual gap: samplerRatio was missing from the Tracing Configuration table in docs/reference/configuration.md, so a reader of that reference wouldn't see it belongs to tracing at all. Added the missing row (with the TRACING_ prefixed env var, matching the other fields) in the latest commit. But since all properties are under type Tracing struct { it's clear they belong to tracing.
| }) | ||
| // liveness probe: reports only that the process is up. It deliberately does not check raft state | ||
| // so that a leaderless node is not restarted in a loop by an orchestrator. | ||
| r.Get("/health/live", func(w http.ResponseWriter, _ *http.Request) { |
There was a problem hiding this comment.
Do we really need this? I would expect this to be part of the status endpoint.
There was a problem hiding this comment.
Yes, this is intentional and distinct from /status:
/system/statusis a verbose diagnostic dump (raw cluster state) that always returns 200 — it's kept stable for existing consumers/tooling that just want to inspect state, and is not meant to be polled by orchestrators./system/health/liveand/system/health/readyreturn the standard boolean pass/fail semantics (200/503) that Kubernetes liveness/readiness probes and load balancers expect out of the box. Folding this into/statuswould force every probe to parse the full cluster-state JSON and re-implement the "am I healthy" logic that's now centralized innode.Health(), and it would also break/status's "always 200" contract for existing consumers if we started returning 503 from it.
Keeping them separate lets /status stay a stable diagnostic/debugging endpoint while /health/live and /health/ready are the ones orchestrators actually wire up.
| }) | ||
| // readiness probe: 503 until the cluster has a leader, every partition | ||
| // has a leader and all partitions owned by this node are initialized. | ||
| r.Get("/health/ready", func(w http.ResponseWriter, _ *http.Request) { |
There was a problem hiding this comment.
Do we really need this? I would expect this to be part of the status endpoint.
There was a problem hiding this comment.
Yes, this is intentional and distinct from /status:
/system/statusis a verbose diagnostic dump (raw cluster state) that always returns 200 — it's kept stable for existing consumers/tooling that just want to inspect state, and is not meant to be polled by orchestrators./system/health/liveand/system/health/readyreturn the standard boolean pass/fail semantics (200/503) that Kubernetes liveness/readiness probes and load balancers expect out of the box. Folding this into/statuswould force every probe to parse the full cluster-state JSON and re-implement the "am I healthy" logic that's now centralized innode.Health(), and it would also break/status's "always 200" contract for existing consumers if we started returning 503 from it.
Keeping them separate lets /status stay a stable diagnostic/debugging endpoint while /health/live and /health/ready are the ones orchestrators actually wire up.
Addresses review comment from @Kuzna: the samplerRatio field already carries the TRACING_ prefix on its env var, but the field was missing entirely from the Tracing Configuration reference table, so readers of docs/reference/configuration.md would not know it existed or belonged to tracing config.
- Fix data race on cluster state read (async metrics callback vs FSM apply) by using an RWMutex and synchronizing all Store.state readers. - Fix job failure-rate metrics: internal job handler failures/completions now record jobs_failed_total / job_lifetime consistently with external jobs. - Record process_instance_duration/processes_completed for message publication failures that persist a terminal instance state outside RunProcessInstance. - Fix timers_scheduled double-counting when a definition-level timer start event is created through an *EngineBatch (event subprocess path). - Record incidents_created_total for event-subprocess subscription incidents saved through a raw storage.Batch. - Make timer/message-correlation metric recorders nil-safe per instrument, since otel.NewMetrics can return a partially initialized struct on error. - Scope node readiness (/system/health/ready) to node-local conditions only; a single degraded remote partition no longer makes every node unready. - Remove the unused go.opentelemetry.io/contrib/instrumentation/runtime dependency; go_* metrics are already exported by the client_golang default collectors on /system/metrics. - Make rqlite db-size metric fail loudly instead of silently reporting 0 when the expected sqlite files are missing/unreadable. - Deduplicate replica-local partition leader-change counters with max by(partition) instead of sum, and stop counting the first post-restart election as a leader change. - Align rqlite/DMN histogram bucket boundaries with the shared LatencyBucketsMs(). Adds regression tests for all of the above.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cluster/partition/partition.go (1)
87-102: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog the metric that actually failed.
The
processInstancesActiveregistration failure is reported as"Failed to register meter for jobsWaiting". This misdirects operators when onlyprocessInstancesActivesetup fails. Change the message to identifyprocessInstancesActive. (raw.githubusercontent.com)Proposed fix
- zpn.logger.Error("Failed to register meter for jobsWaiting", "err", err) + zpn.logger.Error("Failed to register meter for processInstancesActive", "err", err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cluster/partition/partition.go` around lines 87 - 102, Update the error log for the processInstancesActive metric registration to identify processInstancesActive instead of jobsWaiting. Locate the corresponding registration and error-handling block in the partition metrics initialization, leaving the metric setup and other log messages unchanged.
🧹 Nitpick comments (1)
internal/cluster/partition/partition.go (1)
467-471: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
IDfor Go initialisms.Rename
lastObservedLeaderIdandnewLeaderIdtolastObservedLeaderIDandnewLeaderID. This removes the reportedrevivewarnings without changing behavior.Proposed rename
- var lastObservedLeaderId string + var lastObservedLeaderID string - newLeaderId := string(signal.LeaderID) + newLeaderID := string(signal.LeaderID)Update all uses to match the new names.
Also applies to: 519-527
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cluster/partition/partition.go` around lines 467 - 471, Rename the Go variables lastObservedLeaderId and newLeaderId to lastObservedLeaderID and newLeaderID, respectively, in the leader-observation logic and update every reference consistently without changing behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cluster/partition/partition.go`:
- Around line 636-649: Update dbSizeBytes around the os.Stat call to return an
error for any failure that is not os.IsNotExist, rather than continuing with a
partial total; continue skipping only files that disappeared during collection,
while preserving the existing regular-file filtering and zero-file behavior.
In `@pkg/bpmn/jobs.go`:
- Around line 143-157: The terminal-state flow in pkg/bpmn/jobs.go lines 143-157
must persist completed and failed internal jobs before emitting terminal
metrics: after the job.State switch, save the terminal job via batch.SaveJob,
handle any save error, and only record metrics after a successful save. In
pkg/bpmn/engine_metrics_test.go lines 85-106, load the created job after the
failing handler returns and assert its persisted state is
runtime.ActivityStateFailed.
---
Outside diff comments:
In `@internal/cluster/partition/partition.go`:
- Around line 87-102: Update the error log for the processInstancesActive metric
registration to identify processInstancesActive instead of jobsWaiting. Locate
the corresponding registration and error-handling block in the partition metrics
initialization, leaving the metric setup and other log messages unchanged.
---
Nitpick comments:
In `@internal/cluster/partition/partition.go`:
- Around line 467-471: Rename the Go variables lastObservedLeaderId and
newLeaderId to lastObservedLeaderID and newLeaderID, respectively, in the
leader-observation logic and update every reference consistently without
changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b176bf7a-a323-4f42-9ac8-2631d1fb8153
📒 Files selected for processing (22)
docs/reference/configuration.mddocs/reference/observability.mdinternal/cluster/node.gointernal/cluster/node_health_test.gointernal/cluster/partition/partition.gointernal/cluster/partition/partition_metrics_test.gointernal/cluster/partition/partition_persistence.gointernal/cluster/store/fsm.gointernal/cluster/store/fsm_recover_test.gointernal/cluster/store/store.gointernal/cluster/store/store_state_race_test.gointernal/otel/metrics.gointernal/rest/server.gopkg/bpmn/engine.gopkg/bpmn/engine_api.gopkg/bpmn/engine_batch.gopkg/bpmn/engine_metrics_test.gopkg/bpmn/events_api.gopkg/bpmn/jobs.gopkg/bpmn/message_start_event_same_message_test.gopkg/dmn/dmn_engine.goscripts/grafana_provisioning/dashboards/zenbpm/cluster.json
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/rest/server.go
- pkg/bpmn/engine.go
- pkg/dmn/dmn_engine.go
- scripts/grafana_provisioning/dashboards/zenbpm/cluster.json
- pkg/bpmn/events_api.go
- pkg/bpmn/engine_api.go
- internal/cluster/partition/partition_persistence.go
- pkg/bpmn/engine_batch.go
- remove some excessive comments - fix Copilot comments - fix flaky test
Summary by CodeRabbit
New Features
Bug Fixes
Documentation